Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix order creation on PrestaShop V9 #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matthieu-rolland
Copy link
Contributor

Questions Answers
Description? Since PrestaShop V9 an employee id must be set in context if no cart id is set, it makes sense in real use cases, when used in CLI for fixtures we need to set an employee id
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? no
Sponsor company PrestaShop SA
How to test? php bin/console prestashop:shop-creator --orders=10

Comment on lines +31 to +35
// Because we could be in CLI mode, there might be no employee in context, so we must set it manually
$context = Context::getContext();
if (!isset($context->employee) || !isset($context->employee->id)) {
$context->employee = new Employee(1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Because we could be in CLI mode, there might be no employee in context, so we must set it manually
$context = Context::getContext();
if (!isset($context->employee) || !isset($context->employee->id)) {
$context->employee = new Employee(1);
}

This service is not related to the context, and even less about its initialisation It shouldn't be its responsibility to init the legacy context

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a useful tool for that, not very well known or used, that allows the Symfony command to initialize the context instead:
https://github.com/PrestaShop/PrestaShop/blob/d68bfe619ea9ef8e9d57e96ce817028c68f3b885/tests/Integration/PrestaShopBundle/Command/LoadLegacyClassesinCommandTest.php#L123-L124

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthieu-rolland friendly reminder 😉

Copy link
Contributor Author

@matthieu-rolland matthieu-rolland Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matks when using the tool suggested by @jolelievre I saw that it was broken for this use case, so I made a fix on the core

I need this PR to be merged first: PrestaShop/PrestaShop#36358, it's waiting for qa dev

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is now fixed 😄 PrestaShop/PrestaShop#36358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants